Skip to content

Conversation

@zfields
Copy link
Contributor

@zfields zfields commented Jun 2, 2022

This simple abstraction fully isolates the Arduino Stream object, and demonstrates the path to abstraction for both Serial and I2C.

@zfields zfields requested a review from bsatrom June 2, 2022 21:35
@bsatrom bsatrom requested a review from m-mcgowan June 3, 2022 04:22
@zfields zfields marked this pull request as draft June 6, 2022 15:08
@zfields zfields changed the title feat: Full abstraction of NoteLog_Arduino feat: Full abstraction of note-cpp from note-arduino Jun 6, 2022
Copy link
Contributor

@m-mcgowan m-mcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the primary motivation is to abstract the communication and debug streams.

I'd like to summarize the current pattern with NoteLog since NoteSerial follows the same pattern, and begin and setDebugOutputStream also follow a similar pattern, so I'll choose setDebugOutputStream.

Calling Notecard::setDebugOutputStream() follows these steps

  1. The caller passes the arduino Stream to the call. The type is NoteLog::channel_t which is void*

  2. Inside the notecard implementation make_note_log is called, which takes the void* to the stream and creates a NoteLog_Arduino instance, which requires a Stream* - the void* stream is statically cast to Stream*.

  3. NoteLog is a polymorphic interface, with a virtual method print.

The main concerns here is that it isn't type-safe, and there's a tight coupling between the caller and the make_note_log implementation. But thanks to the NoteLog interface, this can be easily fixed in a type-safe way and without the tight coupling. We can also still maintain the singleton instance required too.

#ifdef ARDUINO
// Arduino-specific override
void setDebugOutput(Stream* stream) {
     setDebugOutput(new NoteLog_Arduino(stream));
}
#endif
void setDebugOutput(NoteLog* log) {
    // set the global instance
    note_log = log;
}
private:
   static NoteLog* note_log;

Then when we want to implement this for a different streaming platform, e.g. C stdio, then we can just create a new NoteLog subclass

class NoteLog_stdio : public NoteLog {
    OStream* stream;
    void print(const char* c) {
        stream->write(c);
    }
}

And use that as the debug stream

notecard.setDebugOutput(new NoteLog_stdio(&cout));

This lets the caller choose what type of stream they want, rather than it being fixed by make_note_log and type-safety is provided.

To sum up - the key change is to pass around NoteLog instances rather than the implementation specific NoteLog::channel_t which is merely a catch-all type.

@zfields
Copy link
Contributor Author

zfields commented Jun 8, 2022

  1. Inside the notecard implementation make_note_log is called, which takes the void* to the stream and creates a NoteLog_Arduino instance, which requires a Stream* - the void* stream is statically cast to Stream*.
    ...
    The main concerns here is that it isn't type-safe, and there's a tight coupling between the caller and the make_note_log implementation.

The type safety is the real problem. The make_note_xxx functions declared alongside the interface, and are implemented as static functions in NoteLog_Arduino.cpp, NoteSerial_Arduino and NoteI2c_Arduino, so there is no "tight coupling" so to speak. Part of the original motivation behind the make_note_xxx functions was to provide a way to create a singleton (for note-c), without forcing singleton semantics on note-arduino.

@zfields zfields merged commit 4e37f0b into master Jun 13, 2022
@zfields zfields deleted the zak-note-cpp branch June 13, 2022 14:52
@zfields zfields restored the zak-note-cpp branch June 13, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants